fix(auth): bind internal JWTs to principal secret generation - #5053
Conversation
5925d16 to
1a98a23
Compare
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your diligence, @vigneshio ! The general idea is very reasonable. Some implementation comments below.
1a98a23 to
083f99e
Compare
bba3287 to
a66743e
Compare
0f31cd0 to
e082112
Compare
6158068 to
5377243
Compare
5377243 to
de40af7
Compare
flyrain
left a comment
There was a problem hiding this comment.
Thanks @vigneshio for the PR. I have replied in the private mailing list. I think it's valid to introduce an explicit invalidation operation, but I'm not convinced that we need to invalidate access tokens for principal credential rotating and resetting.
Thanks for the review @flyrain replied in the private thread as well.. PTAL.. The description is now updated to match the actual semantics!!. |
29c2bf5 to
000eb97
Compare
dimas-b
left a comment
There was a problem hiding this comment.
LGTM overall 👍 Some more minor comments.
000eb97 to
a48b81e
Compare
Drop an unrelated CHANGELOG line. Map metastore load failures during token verify to service unavailable instead of NotAuthorized/invalid_client. Reject claim-less legacy tokens on exchange so they cannot be re-minted onto the current credentials generation; bearer verify until expiry is unchanged for upgrade compatibility.
Propagate ServiceUnavailableException from token verify in InternalAuthenticationMechanism so INTERNAL and MIXED auth return 503 instead of treating metastore failures as bad credentials or falling through to external auth. Add unit and HTTP-level coverage.
Shorten the polaris-cv CHANGELOG entry to user-facing behavior only, and use a finite await timeout in InternalAuthenticationMechanismTest.
…_grant, breaking-change note
7015fbc to
eef09e1
Compare
flyingImer
left a comment
There was a problem hiding this comment.
The latest exchange-only rewrite looks sound; I don’t see a code blocker. I have two wording nits so the public contract and rolling-upgrade behavior match the implementation, plus one small stale-test cleanup. Once those are addressed, this looks ready to merge.
|
Given we have 3 approvals on the PR, I'd propose to merge it once CI is happy. |
|
Good points @flyingImer updated the wording/tests to match the exchange-only contract... PTAL.. @dimas-b @flyrain before we merge.
|
|
Thanks for working on it, @vigneshio! Appreciated your patience and persistence! Thanks everyone for the review. |
Bind internal JWT access tokens to their issuing credential generation via a
polaris-cvclaim (no secret material in the token). Verification honors the current and secondary generations, matching the existing dual-secret rotation grace.Single rotations leave active tokens valid; resets (and a second rotation) invalidate prior generations. Token exchange preserves the subject token's generation. Claim-less tokens still verify until expiry for upgrade compatibility, but exchange rejects them. Tokens for deleted principals are rejected because verification requires loadable secrets. Persistence failures while loading secrets during verify return service unavailable rather than an authentication failure. No schema migration is needed.